test(core): make the privacy guards prove what they claim - #935
test(core): make the privacy guards prove what they claim#935ozymandiashh wants to merge 1 commit into
Conversation
|
Back to draft — a final cross-PR review found a critical defect this PR creates in combination with the rest of the batch. Replacing So the first time a JetBrains transcript grows after this lands, the re-parse appends the session's entire history under new keys while the old copies remain, and the daily-cache bumps landing in the same batch then double-count it. A sibling PR established exactly this rule and bumped a parse version for every provider whose key shape it changed. This one changed copilot's and did not. Fix in progress. |
Four guards passed without demonstrating the property they exist for.
**import-smoke stubbed seven modules.** It imports every public entry point
with I/O primitives replaced by throwing stubs, to show that importing core
touches nothing. Global `fetch`, `node:os` (homedir), `node:process` and env,
`node:sqlite`, tls, dgram, http2, worker_threads and `createRequire` — which
bypasses ESM loader hooks entirely — were all open. Environment variables were
emptied rather than made to throw, so a read succeeded silently. It also only
ever proved import purity: decoder bodies were never exercised, so a decoder
that opened a file when called would have passed. Blocklist extended, env made
to throw on ambient keys, and parser, decoder and detector bodies now run under
the same stubs.
**The schema string-field gate walked only part of the schema.** It descended
into properties, items and definitions, so anything under anyOf, oneOf, allOf,
patternProperties, additionalProperties or a union type escaped both the
enumeration and the bound check — and any maxLength at all counted as
"bounded", including one large enough to hold a paragraph.
**The diagnostic sanitiser blocked path separators and nothing called it.**
Its only caller was unused by every decoder.
**The userMessage gate greps one identifier**, so a decoder copying user text
into a differently-named field was invisible to it, and eight allowlisted
providers had no hostile-decode test at all. They have one now.
None of the four original guards failed against current code — but review of
the hardened surface found two live holes, both closed in this commit (below).
The green tests now say something true.
**Review, round one — two live holes the hardening missed, now closed.**
- isolateRecords forwarded caller-supplied diagnostics unchanged, and
RecordOutcome permitted a `detail`, so a caller returning a diagnostic could
emit an unkeyed fingerprint through a public entry point. RecordOutcome now
carries `{ index, code }` only, and the runtime strips any detail a caller
smuggles in (a loose cast cannot smuggle one past the runtime). The
fingerprint is derived in exactly one place: isolateRecords itself, from a
thrown error, under the key it owns.
- the copilot JetBrains dedup key was an unkeyed sha256 of the assistant reply
text, truncated to 12 hex — 48 bits, dictionary-attackable on short replies
like "OK" or "Done." — and it crosses into the emitted envelope
(observations.dedupKey) and the CLI ledger. It is now an HMAC keyed with the
host's privacy key. The key IS available: decodeCopilot receives `context`,
and the digest stays deterministic across re-parses, which is what the
durable-store dedup needs. One caveat, documented at the site: the CLI's
bridged rich-decode path passes an empty privacy key, so on that path the
digest is only as strong as an empty key — closing it fully needs the CLI to
hand the rich decode a real privacy key (getHostPrivacyKey() exists on its
sync path), which is the adjacent CLI change. The CLI bridge goldens that
pinned the unkeyed digests were recomputed against the keyed construction.
The value of EVERY cached dedup key changed, not just hostile ones, so the
copilot entry in PROVIDER_PARSE_VERSIONS is bumped in lockstep
(cli-shutdown-cost-v1-skills -> ...-dedup-key-hmac-v1): copilot is the sole
durable provider, whose union-merge appends any turn whose keys are not
already cached and never deletes, and the env-fingerprint parse-version bump
is the only mechanism that drops old-shape keys. Without it, the first
JetBrains transcript re-parse after this lands would append the session's
entire history under the new keys while the old copies remain — both coexist,
and the daily cache re-derives double-counted copilot totals. The bump makes
the section rebuild once, dropping the old keys instead of merging.
**Review, round one — corrections to the guards themselves.**
- The open-design and lingtai-tui smuggling fixtures planted secrets in fields
their decoders never read, proving only that ignored fields stay ignored;
they now plant in the token fields those decoders genuinely read and assert
the hostile payload was coerced away by the read itself, never echoed.
- The zed error path reads context defensively (it is the only place the
function touches context; an untyped caller gets a diagnostic, not a
TypeError).
- The import-smoke env probe sets a guaranteed-present sentinel key instead of
relying on HOME, so the guard tests what it means even in a HOME-less CI
container.
- The architecture-gate rationale no longer claims a 256 cap rules out short
tokens (a GitHub PAT is ~93 chars); it is an anti-free-text bound, with the
anti-credential guarantee resting on the content-smuggling tests.
- The dead `byteOffset` field was removed from RecordDiagnostic — nothing
emitted it.
One thing this change nearly got wrong, worth recording. Wiring the sanitiser
into the decoders introduced a `detail` fingerprint where diagnostics had been
bare — and the sanitiser's key was optional, silently degrading to an unkeyed
digest. The bridge decodes with an empty privacy key, so on the real path that
digest would have been unkeyed, derived from an error message, and Node embeds
a fragment of the offending input in JSON.parse failures. That is precisely the
dictionary-attackable construction D1 exists to prevent, added by the change
meant to close it. The key is now required and throws like fingerprint.ts does;
decoders route through a helper that omits the field entirely when no key is
available, because a bare throw there would break poison-isolation and take the
whole batch down with one malformed record. Four regression tests fail if the
keyless path returns. The copilot JetBrains dedup key was a second instance of
the same construction that survived the first pass — an unkeyed digest of
reply text crossing the boundary — and is keyed now.
0b1353d to
612da79
Compare
Four guards passed without demonstrating the property they exist for. Two turned out to be hiding live holes.
What was wrong
import-smoke stubbed seven modules. It imports every public entry point with I/O replaced by throwing stubs, to show that importing core touches nothing. Global
fetch,node:os(homedir),node:processand env,node:sqlite, tls, dgram, http2, worker_threads andcreateRequire— which bypasses ESM loader hooks entirely — were all open. Env was emptied rather than made to throw, so a read succeeded silently. It also only proved import purity: decoder bodies never ran, so a decoder that opened a file when called would have passed. Blocklist extended, env made to throw, and parser, decoder and detector bodies now execute under the same stubs.The schema string-field gate walked only part of the schema — properties, items, definitions — so anything under
anyOf,oneOf,allOf,patternPropertiesor a union type escaped both the enumeration and the bound check.The diagnostic sanitiser blocked path separators and had no live caller.
The userMessage gate greps one identifier, so a decoder copying user text into a differently-named field was invisible, and eight allowlisted providers had no hostile-decode test.
Two live holes closed, found by review
isolateRecordslet unkeyed detail through the public API. Only thrown errors were protected; a caller returning a diagnostic could still emit an unkeyed fingerprint.The copilot JetBrains decode built a dedup key from an unkeyed SHA-256 of assistant reply text, truncated to 48 bits — dictionary-attackable on short replies like "OK" or "Done." — and it was copied into the emitted envelope. Investigation showed the key is available in that decode context, so it is now HMAC-keyed. The bridged CLI path still passes an empty key, so full closure needs a real key threaded into rich decode; that constraint is documented at the site. The bridge goldens that pinned the unkeyed digests were recomputed with the real decoder rather than hand-edited.
The near-miss worth recording
Wiring the sanitiser into decoders added a
detailfingerprint where diagnostics had been bare — and the sanitiser's key was optional, degrading silently to unkeyed. The bridge decodes with an empty key, so on the real path that digest would have been unkeyed, over an error message, and Node embeds a fragment of the offending input inJSON.parsefailures. A hostile blob could have smuggled content out through the field added to prevent smuggling.The key is now required and throws like
fingerprint.tsdoes. Decoders route through a helper that omits the field entirely when no key exists — a bare throw there would break poison-isolation and take a whole batch down with one malformed record. Four regression tests fail if the keyless path returns.Also
Two hostile tests planted secrets in fields their decoders never read, which proved only that ignored fields stay ignored while looking like coverage; they now plant in genuinely-read fields. The env probe uses an explicit sentinel instead of
HOME, so it cannot false-red on a HOME-less runner. The 256-char bound's rationale is corrected — it is anti-free-text, not anti-credential, since a GitHub PAT is ~93 characters. DeadbyteOffsetsurface removed.556 core tests green, typecheck clean.